Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add extensions to graphql span events on errors #12181

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

feat: add extensions to graphql span events on errors #12181

wants to merge 16 commits into from
+403 −36

Conversation

quinna-h
Copy link
Contributor

@quinna-h quinna-h commented Jan 30, 2025 •
edited
Loading

An extension to: feat: add span events on graphql error spans on 3.x

Add user-provided extensions according to RFC, update tests and add snapshots.

Note: some files / methods in the diff will be removed once #12224 is merged. Including these were necessary to allow tests to run.

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

@quinna-h quinna-h changed the title add extensions to graphql span events on errors feat: add extensions to graphql span events on errors Jan 30, 2025
@pr-commenter
Copy link

pr-commenter bot commented Jan 30, 2025 •
edited
Loading

Benchmarks

Benchmark execution time: 2025-02-10 19:31:36

Comparing candidate commit 2215d7d in PR branch quinna.halim/add-error-extensions-graphql with baseline commit a604d93 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 394 metrics, 2 unstable metrics.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 30, 2025 •
edited
Loading

CODEOWNERS have been resolved as:

releasenotes/notes/graphql-error-span-events-add-extensions-5eece423cc8ff93e.yaml  @DataDog/apm-python
tests/snapshots/tests.contrib.graphene.test_graphene.test_schema_failing_extensions.json  @DataDog/apm-python
tests/snapshots/tests.contrib.graphql.test_graphql.test_graphql_fail.json  @DataDog/apm-python
ddtrace/contrib/internal/graphql/patch.py                               @DataDog/apm-core-python @DataDog/apm-idm-python
tests/contrib/graphene/test_graphene.py                                 @DataDog/apm-core-python @DataDog/apm-idm-python
tests/contrib/graphql/test_graphql.py                                   @DataDog/apm-core-python @DataDog/apm-idm-python
tests/snapshots/tests.contrib.graphql.test_graphql.test_graphql_error.json  @DataDog/apm-python

quinna-h
quinna-h commented Feb 4, 2025
View reviewed changes
tests/contrib/graphql/test_graphql.py


@snapshot(ignores=["meta.error.type", "meta.error.message", "meta.events"])
def test_graphql_fail(enable_graphql_resolvers):
Copy link
Contributor Author

@quinna-h quinna-h Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated from #12224, will be removed after that PR is merged

quinna-h
quinna-h commented Feb 4, 2025
View reviewed changes
tests/snapshots/tests.contrib.graphql.test_graphql.test_graphql_error.json Outdated
Copy link
Contributor Author

@quinna-h quinna-h Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File will be removed from PR once #12224 is merged

@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Feb 4, 2025 •
edited
Loading

Datadog Report

Branch report: quinna.halim/add-error-extensions-graphql
Commit report: b7fe47f
Test service: dd-trace-py

✅ 0 Failed, 130 Passed, 1378 Skipped, 5m 2.51s Total duration (35m 27.35s time saved)

quinna-h
quinna-h commented Feb 4, 2025
View reviewed changes
tests/snapshots/tests.contrib.graphql.test_graphql.test_graphql_error.json Outdated
Copy link
Contributor Author

@quinna-h quinna-h Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed after #12224

quinna-h
quinna-h commented Feb 5, 2025
View reviewed changes
ddtrace/contrib/internal/graphql/patch.py
error_extensions[field] = error.extensions[field]
else:
# q: could this be `None`?
error_extensions[field] = str(error.extensions[field])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could error.extensions[field] ever be None?

@quinna-h quinna-h force-pushed the quinna.halim/add-error-extensions-graphql branch from b7fe47f to a84739d Compare February 6, 2025 17:27
@quinna-h quinna-h changed the base branch from main to 3.x-staging February 6, 2025 17:28
@quinna-h quinna-h marked this pull request as ready for review February 6, 2025 19:04
@quinna-h quinna-h requested review from a team as code owners February 6, 2025 19:04
@erikayasuda erikayasuda changed the base branch from 3.x-staging to main February 6, 2025 19:10
@erikayasuda erikayasuda requested review from a team as code owners February 6, 2025 20:43
@emmettbutler emmettbutler force-pushed the quinna.halim/add-error-extensions-graphql branch from 246fa84 to 2215d7d Compare February 10, 2025 18:01
@emmettbutler emmettbutler mentioned this pull request Feb 10, 2025
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmettbutler emmettbutler emmettbutler approved these changes

@taegyunkim taegyunkim Awaiting requested review from taegyunkim taegyunkim is a code owner automatically assigned from DataDog/apm-python

@wconti27 wconti27 Awaiting requested review from wconti27 wconti27 is a code owner automatically assigned from DataDog/apm-python

@erikayasuda erikayasuda Awaiting requested review from erikayasuda erikayasuda is a code owner automatically assigned from DataDog/apm-core-python

@mabdinur mabdinur Awaiting requested review from mabdinur mabdinur was automatically assigned from DataDog/apm-sdk-api-python

@tonyredondo tonyredondo Awaiting requested review from tonyredondo tonyredondo was automatically assigned from DataDog/ci-app-libraries

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@quinna-h @emmettbutler